Conversation
📝 WalkthroughWalkthroughThis PR introduces a new performer-scoped endpoint to retrieve created amateur shows with ownership validation, updates the runtime field type from String to Integer across DTOs and entities, adds approval status checking to the existing show retrieval endpoint, and introduces a new error status for non-approved shows. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/cc/backend/amateurShow/controller/AmateurController.java (1)
104-109:⚠️ Potential issue | 🟡 MinorPre-existing bug:
getShowIncomingdelegates togetShowToday.Line 107 calls
amateurService.getShowToday(pageable)instead of what should likely be a dedicatedgetShowIncomingmethod. This appears to be a copy-paste error from the/todayendpoint. Not introduced by this PR, but worth noting.
🤖 Fix all issues with AI agents
In `@src/main/java/cc/backend/amateurShow/controller/AmateurController.java`:
- Around line 63-70: The service method handling getCreatedShow in
AmateurServiceImpl currently throws ErrorStatus.MEMBER_NOT_PERFORMER when a show
isn't found or isn't owned by the authenticated performer; change that to a
semantically correct status such as ErrorStatus.SHOW_NOT_FOUND or
ErrorStatus.SHOW_NOT_OWNED_BY_MEMBER in the code path that checks
ownership/fetch result (the branch that currently throws MEMBER_NOT_PERFORMER
around the getCreatedShow handling), and update any related exception
construction or messages so the controller AmateurController.getCreatedShow
receives the correct error code.
In `@src/main/java/cc/backend/amateurShow/converter/AmateurConverter.java`:
- Line 274: The builder call in AmateurConverter that sets
.runtime(amateurShow.getRuntime() + "분") will produce "null분" when
amateurShow.getRuntime() is null; change it to conditionally format the value
(e.g., if amateurShow.getRuntime() == null then pass null or an empty string,
otherwise pass amateurShow.getRuntime() + "분") so the runtime field is
omitted/empty instead of "null분"; locate the .runtime(...) invocation in
AmateurConverter and replace the direct concatenation with a null-safe
conditional/ternary or use Optional to format only when getRuntime() is
non-null.
🧹 Nitpick comments (1)
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java (1)
457-464: Consider using a more precise error when the show is simply not found.
findByIdAndMemberIdreturns empty for two distinct cases: the show doesn't exist at all, or it belongs to a different member. ThrowingMEMBER_NOT_PERFORMERfor both conflates "not found" with "not authorized." This could confuse the creator if they pass an invalid show ID—they'd receive a permissions error instead of a 404.A two-step lookup (find by ID, then verify ownership) would let you return
AMATEURSHOW_NOT_FOUNDvs.MEMBER_NOT_AUTHORIZEDas appropriate.♻️ Proposed refactor
`@Override` public AmateurShowResponseDTO.AmateurShowResult getCreatedShow(Long memberId, Long amateurShowId){ - AmateurShow amateurShow = - amateurShowRepository.findByIdAndMemberId(amateurShowId, memberId) - .orElseThrow(() -> new GeneralException(ErrorStatus.MEMBER_NOT_PERFORMER)); + AmateurShow amateurShow = amateurShowRepository.findById(amateurShowId) + .orElseThrow(() -> new GeneralException(ErrorStatus.AMATEURSHOW_NOT_FOUND)); + + if (!amateurShow.getMember().getId().equals(memberId)) { + throw new GeneralException(ErrorStatus.MEMBER_NOT_AUTHORIZED); + } return AmateurConverter.toResponseDTO(amateurShow); }
| @PreAuthorize("hasRole('PERFORMER')") | ||
| @GetMapping("/created/{amateurShowId}") | ||
| @Operation(summary = "방금 등록한 공연 단건 조회(아직 미승인) - 등록자용") | ||
| public ApiResponse<AmateurShowResponseDTO.AmateurShowResult> getCreatedShow( | ||
| @AuthenticationPrincipal(expression = "member") Member member, | ||
| @PathVariable Long amateurShowId){ | ||
| return ApiResponse.onSuccess(amateurService.getCreatedShow(member.getId(), amateurShowId)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
rg -n "getCreatedShow" --type=java -A 15Repository: SeeATheater/CC_Backend
Length of output: 5440
Fix misleading error code in service layer.
The endpoint is properly secured with @PreAuthorize("hasRole('PERFORMER')") and correctly extracts the authenticated member. However, the service throws ErrorStatus.MEMBER_NOT_PERFORMER when the show isn't found for that member (line 461 in AmateurServiceImpl.java). This error code is semantically incorrect — the member is a performer (they passed the role check); the real issue is the show doesn't exist or isn't owned by that member. Use SHOW_NOT_FOUND or SHOW_NOT_OWNED_BY_MEMBER instead.
🤖 Prompt for AI Agents
In `@src/main/java/cc/backend/amateurShow/controller/AmateurController.java`
around lines 63 - 70, The service method handling getCreatedShow in
AmateurServiceImpl currently throws ErrorStatus.MEMBER_NOT_PERFORMER when a show
isn't found or isn't owned by the authenticated performer; change that to a
semantically correct status such as ErrorStatus.SHOW_NOT_FOUND or
ErrorStatus.SHOW_NOT_OWNED_BY_MEMBER in the code path that checks
ownership/fetch result (the branch that currently throws MEMBER_NOT_PERFORMER
around the getCreatedShow handling), and update any related exception
construction or messages so the controller AmateurController.getCreatedShow
receives the correct error code.
| .posterImageUrl(amateurShow.getPosterImageUrl()) | ||
| .schedule(schedule) | ||
| .runtime(amateurShow.getRuntime()) | ||
| .runtime(amateurShow.getRuntime() + "분") |
There was a problem hiding this comment.
Null runtime will produce the string "null분".
amateurShow.getRuntime() returns Integer (nullable). When null, the concatenation null + "분" yields "null분" instead of an empty or omitted value.
🐛 Proposed fix
- .runtime(amateurShow.getRuntime() + "분")
+ .runtime(amateurShow.getRuntime() != null ? amateurShow.getRuntime() + "분" : null)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .runtime(amateurShow.getRuntime() + "분") | |
| .runtime(amateurShow.getRuntime() != null ? amateurShow.getRuntime() + "분" : null) |
🤖 Prompt for AI Agents
In `@src/main/java/cc/backend/amateurShow/converter/AmateurConverter.java` at line
274, The builder call in AmateurConverter that sets
.runtime(amateurShow.getRuntime() + "분") will produce "null분" when
amateurShow.getRuntime() is null; change it to conditionally format the value
(e.g., if amateurShow.getRuntime() == null then pass null or an empty string,
otherwise pass amateurShow.getRuntime() + "분") so the runtime field is
omitted/empty instead of "null분"; locate the .runtime(...) invocation in
AmateurConverter and replace the direct concatenation with a null-safe
conditional/ternary or use Optional to format only when getRuntime() is
non-null.
1 participant
#이슈번호,#이슈번호Summary by CodeRabbit
Release Notes
New Features
Improvements